gas(contracts): cache repeated storage/external reads in hot paths - #259

Reviewed against develop in an isolated worktree (~/tempdir/DIN/PRs/PRreview_259, branch pr-259-review, 1 commit, no conflicts — confirmed by both git merge-tree --write-tree locally and GitHub's own mergeable: MERGEABLE). Merge base 6e361cb0; the branch is 8 commits behind the current develop tip (eb8085da), all on unrelated files — clean merge confirmed by local dry run.

Claimed (finding 1): Registration (registerDINaggregator / registerDINAuditor) called getStake(msg.sender) up to twice. The fix shares a single call behind a floorMin > 0 || capPerUnit > 0 guard so the common case (both bounds unset) makes zero stake reads.

Verified — identical pattern on both contracts, behavior preserved: The DINTaskCoordinator and DINTaskAuditor diffs are symmetric: both replace the two independent getStake calls with one local stake variable read only when at least one bound is set. The floorMin > 0 and capPerUnit > 0 conditions are logically equivalent to the originals; no revert path is added or removed. CI Solidity (4m49s, 484 passed, 0 failed, via_ir) at https://github.com/InfiniteZeroFoundation/DevNet/actions/runs/38715584516/job/116357345649 confirms no behavioral regression.

Claimed (finding 2): _activeAggregatorPool / _activeAuditorPool called isValidatorActive() once in the counting pass and again in the fill pass — two external calls per validator. The fix caches each result into a bool[] memory and reuses it.

Verified — cache correctly parallels the counting loop: Both helpers now declare bool[] memory active = new bool[](len) sized by the pre-read len = registeredPool.length. The counting loop writes active[i] = isActive in the same pass that increments activeCount; the fill loop reads active[i] without touching dinvalidatorStakeContract again. The output is identical: only validators that were active at the start of the call appear in the pool. Gas saving is proportional to the validator pool size; no storage write depends on these reads so caching is safe.

Claimed (finding 3): finalizeT1Aggregation, slashAggregators, finalizeEvaluation, slashAuditors re-read batchId / .length fields from storage on every loop iteration.

Verified — all four sites correctly hoisted: Read each diff hunk. finalizeT1Aggregation: batchCount hoisted above the batch loop; batchId and aggregatorCount hoisted to the top of each batch iteration before their inner loops. slashAggregators: t1Count hoisted above the T1 loop. finalizeEvaluation: modelIndexCount and auditorCount hoisted per batch (were re-read inside the nested model × auditor loops — the most impactful hoist). slashAuditors: same two hoists. None of these fields changes within a batch loop body, so hoisting is safe.

Claimed (finding 4): _syncValidatorStatus reads validator.status twice and validator.activeStake twice from the same ValidatorInfo storage slot group.

Verified — cache is correct: Both reads are replaced with local ValidatorStatus status and uint256 activeStake. The logic is unchanged: the same branch conditions appear in the same order; only the storage slot reads are deduplicated.

Claimed (finding 5): releaseGIRegistrationSlots' two string requires become TC_GINotEnded / TC_SlotsAlreadyReleased custom errors.

Verified — errors added to DINShared.sol and used correctly. Critical overlap with PR No. 272: PR No. 272 (richer GI states, currently open and approved) makes the identical change — it also adds TC_GINotEnded and TC_SlotsAlreadyReleased to DINShared.sol and rewrites releaseGIRegistrationSlots to use them, for the same −88 B size reason. Confirmed by diffing both branches' DINShared.sol additions. Whichever of these two PRs merges second will need to strip the 5-line error block from DINShared.sol and the 2 changed lines in DINTaskCoordinator.releaseGIRegistrationSlots, plus the two new error imports in StakingEnforcement.t.sol — those changes will already be on develop from the first merge. The dincli/abis/DINTaskCoordinator.json additions for these two errors will also need to come only from one PR. The gas-caching hunks (findings 1–4) are fully independent and do not overlap.

Claimed: Size gate: DINTaskCoordinator −195 B (23,302 B, warn, same band as develop), DINTaskAuditor +22 B (21,312 B, ok).

Verified — CI size gate confirms: CI Solidity passes the gate. The warn band on DINTaskCoordinator is pre-existing, consistent with develop.

Every gas-caching claim (findings 1–4) held up — the hoists are safe and the behavior is unchanged. The critical coordination point is finding 5's overlap with PR No. 272: one of the two PRs will need to drop those lines before or after rebasing. All checkable claims hold. Looks good to merge once the second sign-off is in place and the PR No. 272 sequencing is decided.

Files changed (5) — as of e20267fb (PR head, post-rebase onto develop @ 55c4b52b)

Diffed against current develop tip 55c4b52b. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: BEHIND (develop moved 2 unrelated commits since the rebase). Local git merge-tree --write-tree dry run clean (exit 0).

foundry/src/DINTaskCoordinator.sol

foundry/src/DINShared.sol

foundry/src/DINTaskAuditor.sol

foundry/src/DinValidatorStake.sol

foundry/test/StakingEnforcement.t.sol

Verification

CI OK, Docs, Python (1m19s), Solidity (6m58s, 509 passed, 0 failed, via_ir) all green on e20267fb at https://github.com/InfiniteZeroFoundation/DevNet/actions/runs/40182525721. Gas evidence in PR body (DINTaskCoordinator finalizeT1Aggregation −3,056 avg gas, slashAggregators −1,819) is plausible given the hoists; local gas-report re-run not required per review-depth rule. Local merge-tree dry run clean.

Note: second sign-off from @umeradl or @abrahamnash required (PR touches foundry/src/**; no mechanism change so @umeradl may provide it).

Local vs. GitHub agree: both report a clean merge.

Rebased onto current develop (now includes PR #272, which merged the identical TC_GINotEnded/TC_SlotsAlreadyReleased addition to DINShared.sol and the same releaseGIRegistrationSlots change you flagged as the coordination point).

Resolved as predicted: dropped my duplicate TC_GINotEnded/TC_SlotsAlreadyReleased declarations from DINShared.sol (kept develop's copy from #272), kept my aggCount caching on top of the shared releaseGIRegistrationSlots body, and resolved the one real conflict marker in StakingEnforcement.t.sol's import block (kept TC_GINotEnded there since my revertsWhenGINotEnded test still needs it - #272 never added that import/test).

Rebuilt clean, regenerated both ABIs (matched byte-for-byte against a fresh regen, no stale entries left over), full suite 509/509, doc links clean. Size gate still warn (same band as develop), margin actually improved slightly post-merge since the duplicate error block is gone.

Findings 1-4 (the actual gas-caching, independent of #272) are untouched by any of this.

Re-reviewed fix commits: 283c66fc (previously reviewed) → e20267fb (current head after rebase onto develop, which now includes PR No. 272).

The gas-caching hunks (findings 1–4) are untouched — no new commits changed those. This re-review covers only the rebase resolution for finding 5.

Finding 5 — rebase resolution verified

The predicted conflict materialised exactly as described in the merge-proposal comment. Confirmed each resolution point:

1. DINShared.sol — no duplicate error declarations. The PR removes PR No. 272's shorter NatSpec version (/// @dev releaseGIRegistrationSlots: the GI has not ended yet.) and replaces it with a more precise description (releaseGIRegistrationSlots was called for a GI that has not ended yet (not < current GI, and not == current GI with state GIended)), repositioned after TC_GIRewardPoolNotFunded. Error names and signatures are identical — no ABI impact. Net vs develop: +5/−4.

2. DINTaskCoordinator.sol — releaseGIRegistrationSlots body is intact. aggCount caching (finding 3 addition to this function) is correctly layered on top of develop's TC_GINotEnded/TC_SlotsAlreadyReleased reverts. Net vs develop: +42/−27.

3. foundry/test/StakingEnforcement.t.sol — conflict resolved correctly. Develop (PR No. 272) already had TC_SlotsAlreadyReleased in the import block and rewrote the double-release test to use .selector. PR No. 259 adds only TC_GINotEnded to the import (line 25, verified present) and test_releaseGIRegistrationSlots_revertsWhenGINotEnded (line 493, tests the before-GI-end revert path that PR No. 272 never covered). Net vs develop: +10/−0.

4. dincli/abis/DINTaskCoordinator.json — now has no diff vs develop. PR No. 272 already added the TC_GINotEnded and TC_SlotsAlreadyReleased ABI entries; this file requires no further change from PR No. 259. (The previous merge-proposal entry for this file is now moot.)

CI and merge status

CI OK, Docs, Python (1m19s), Solidity (6m58s, 509 passed, 0 failed, via_ir) all green on e20267fb at https://github.com/InfiniteZeroFoundation/DevNet/actions/runs/40182525721. Local git merge-tree --write-tree clean against current develop tip (55c4b52b). GitHub: mergeable: MERGEABLE, mergeStateStatus: BEHIND (develop moved 2 commits since the rebase — neither file overlaps).

All findings verified. The PR No. 272 coordination point is fully resolved. Second sign-off from @umeradl or @abrahamnash still required (foundry/src/** touched).

PR #272 got reverted (#273, process reason - missing non-author sign-off, content not disputed), which undid the exact TC_GINotEnded/TC_SlotsAlreadyReleased addition I'd deferred to on the last rebase. Fixed:

• Restored my own TC_GINotEnded/TC_SlotsAlreadyReleased declarations in DINShared.sol (develop no longer has them at all now that feat(task-coordinator): richer GI states — AuditSeedLocked, AuditTestDataAssigned, AggSeedLocked (task_061026_21 A+B) #272's reverted) and the custom-error reverts in releaseGIRegistrationSlots - the auto-merge had quietly fallen back to the string requires, which would've broken the revertsWhenGINotEnded test since it still expected the custom error.
• Restored TC_SlotsAlreadyReleased.selector in the double-release test (same reason).
• Also picked up a separate develop-refresh robertocarlous had pushed directly to this branch in the meantime (PR docs(contracts): document one-shot dispute resolution rule and add regression tests (BL-15) #264 + PR feat(stake): cross-model S5 escalation (#193) #236's cross-model S5 work) - merged that in too, no conflicts there.

Rebuilt clean, regenerated all three touched ABIs (Coordinator/Auditor/ValidatorStake - all matched a fresh regen byte-for-byte, nothing stale), full suite 517/517, doc links clean. Size gate back to the original numbers (23,302B / 1,274B margin on the coordinator) now that the duplicate-error situation is gone entirely.

Re-verification — head 6493426d

Following Similoluwa's rebase after No. 273 landed.

Peer check result: python3 .claude/scripts/din_peer_check.py pr 259 → REFUSE. Commit 577e6c7 ("Merge branch 'develop' into fix/issue-249-gas-caching") on the branch has robertocarlous as author — a develop-refresh push made during an earlier review cycle. I cannot approve.

Who should approve: @umeradl or @abrahamnash — both eligible per MAINTAINING No. 3. foundry/src/ is touched, so the second-key requirement also applies (MAINTAINING No. 4.2); one approval from either satisfies both the peer-check gate and the second-key requirement simultaneously.

Files changed (6) — as of 6493426d (PR head)

Diffed against merge-base ee996199 (develop). Develop has moved forward since the branch was cut (No. 272 landed, then No. 273 reverted it); the only overlap affecting this PR was the removal of TC_GINotEnded/TC_SlotsAlreadyReleased in No. 273 — Similoluwa resolved that by redeclaring both errors in DINShared.sol as PR 259's own. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: BLOCKED (pending approval only — no code conflicts). Local git merge-tree --write-tree confirms clean.

foundry/src/DINTaskCoordinator.sol

foundry/src/DINTaskAuditor.sol

foundry/src/DinValidatorStake.sol

foundry/src/DINShared.sol

foundry/test/StakingEnforcement.t.sol

dincli/abis/DINTaskCoordinator.json

Verification

forge test on head 6493426d: 416 passed, 30 UpgradeValidation failures — all pre-existing on develop (stale build-info, confirmed independently). ABI byte-identity check passed. Merge-tree clean. Full detail in the verification comment above.

Local vs. GitHub agree: both report no code conflicts; GitHub BLOCKED is approval-only, not a conflict.

@umeradl everything checks out well, i have added the review details, we can go ahead and approve this PR if this checks out well with you

Closing keyword: Closes #249 is one function short.

We're about to make develop the default branch, so from now on Closes #249 closes issue No. 249 automatically when this PR merges (Developer/MAINTAINING.md No. 4.3).

Finding 3 in the issue lists finalizeT2Aggregation ("same shape") next to finalizeT1Aggregation, but this PR doesn't touch it. On develop, foundry/src/DINTaskCoordinator.sol finalizeT2Aggregation (from :1090) still re-reads batches.length, b.aggregators.length and b.batchId on every loop pass, the same pattern this PR caches in finalizeT1Aggregation.

Either option is fine:

1. Apply the same caching to finalizeT2Aggregation (and add its before/after to the gas table and the size-gate table), so Closes #249 holds; or
2. Change the PR body to Part of #249 and say that finalizeT2Aggregation is left for a follow-up.

Good catch — pushed bec5dd0 applying the identical caching to finalizeT2Aggregation (same batches.length / b.batchId / b.aggregators.length re-reads finding 3 called out, same fix as finalizeT1Aggregation, same comment).

Gas (AggregatorCommitRevealTest + LifecycleEventsTest + StakingEnforcementTest, 9 calls): avg 130,060 -> 129,671 (-389), max 206,468 -> 205,761 (-707). Added to the gas table.

Size: DINTaskCoordinator runtime 23,302 B -> 23,295 B, margin improves to 1,281 B (still warn, not fail). Added to the size table.

forge test: 517 passed, 0 failed.

Kept Closes #249 — this closes the gap finding 3 left open, so the keyword should hold now.

Merged develop at ae7e8d6 to clear the conflict (837ce49). GitHub now reports MERGEABLE (BLOCKED is approval-only).

The conflict was in foundry/test/StakingEnforcement.t.sol over the new TC_GINotEnded import. DINShared.sol auto-merged but duplicated TC_GINotEnded/TC_SlotsAlreadyReleased — they now already exist on develop from the re-landed GI-states work, so I removed this PR's copies and kept develop's declarations. The PR diff against develop is now just the four intended files.

Verification on the merge commit: forge clean && forge build OK, forge test → 526 passed, 0 failed (the earlier 32 UpgradeValidation failures were the stale partial build-info; a clean rebuild clears them, as expected on develop). dincli/abis/DINTaskCoordinator.json is byte-identical to the fresh forge build output.

Re-review: 6493426 (Oct 26 head) → bec5dd0 (Nov 2) → 837ce49 (current HEAD, merges develop ae7e8d6 at 16:10 UTC today). I reviewed the full foundry/src diff at 837ce49 against merge-base ae7e8d6: 4 files, +110/−63. git merge-tree --write-tree exits 0, and GitHub agrees (mergeable: MERGEABLE). CI on 837ce49: CI OK green: Solidity forge test 526 passed, 0 failed (40 suites), size gate included; Python and Docs green.

My Oct 29 point (closing keyword / finalizeT2Aggregation)

Fixed in bec5dd0. finalizeT2Aggregation now caches batches.length, b.batchId and b.aggregators.length exactly like finalizeT1Aggregation. Every remaining b.batchId read inside the loop has been replaced: t2Submitted/t2SubmissionCID/t2Votes/tier2FinalizedAt all use batchId. No write to b.batchId or b.aggregators happens inside the loop, so the cache is safe.

PR No. 287 overlap (resolved by 837ce49)

PR No. 287, merged today as ae7e8d6, already contained this PR's finding 5: the TC_GINotEnded/TC_SlotsAlreadyReleased custom errors in DINShared.sol, plus the releaseGIRegistrationSlots conversion and its ABI entries. Before 837ce49, git auto-merged DINShared.sol with both declarations, and I reproduced that locally on a trial merge of bec5dd0 with develop: forge build → Error (2333): Identifier already declared at DINShared.sol:366 / :368. Git doesn't flag this as a conflict, because the hunks are disjoint. 837ce49 resolves it correctly: DINShared.sol and dincli/abis/DINTaskCoordinator.json are no longer in the diff, so one declaration each remains (DINShared.sol:315, :317, from develop). StakingEnforcement.t.sol keeps PR No. 287's assignPlaceholderTestData alongside this PR's TC_GINotEnded import and not-ended test. My own resolution of the same trial merge was identical, and gave forge test 526 passed, 0 failed on a clean build.

Findings 1–4: each checked for unchanged behaviour

Size (amendment 1): on my trial merge, DINTaskCoordinator is 23,393 B (1,183 B margin) vs develop ae7e8d6 23,507 B (1,069 B): −114 B, still warn, not fail. DINTaskAuditor is 21,319 B (3,257 B margin). CI's size gate on 837ce49 reports the same numbers (Solidity job).

Non-blocking

1. PR body is stale after 837ce49. Finding 5 now lands through PR No. 287, not this PR. The size table is from before 837ce49 (23,295 B vs 23,497 B baseline), and the "Conflicts" list still names PR No. 236, which has merged. Please update the body. Closes #249 still holds, because findings 1–4 are here and finding 5 is on develop.
2. Amendment 4 gas evidence has no finalizeEvaluation row. It's the function where finding 3 removes the most re-reads (modelIndexes.length inside the nested loop). A one-line before/after would complete the table.
3. PR No. 253 (amendment 2). Finding 2 optimises _activeAggregatorPool/_activeAuditorPool, and PR No. 253 removes their only callers. Whichever lands second should delete the helpers, as I've asked on PR No. 253.
4. The Oct 26 merge-proposal table above doesn't describe this PR. It lists "transient-cache getters seedHash()/epochHash()/batchRoot()", which aren't in the diff at any head. My merge-proposal comment below replaces it.

Every finding holds up as behaviour-preserving caching, the overlap with PR No. 287 is resolved correctly, and the build, tests and size gate pass. Approving at 837ce49.

Files changed (4) — as of 837ce49 (PR head)

Diffed against merge-base ae7e8d6, which is current develop (the author merged it in at 16:10 UTC today). The branch is up to date. GitHub agrees: mergeable: MERGEABLE. Confirmed with a local git merge-tree --write-tree dry run: clean. This replaces the Oct 26 table above, which describes code that isn't in this PR.

foundry/src/DINTaskCoordinator.sol

foundry/src/DINTaskAuditor.sol

foundry/src/DinValidatorStake.sol

foundry/test/StakingEnforcement.t.sol

Verification

forge test on my trial merge (bec5dd0 + develop, resolved the same way as 837ce49), after forge clean: 526 passed, 0 failed. Contract size gate: DINTaskCoordinator 23,393 B / 1,183 B margin (warn), DINTaskAuditor 21,319 B. CI on 837ce49 matches: 526 passed, same sizes (run 42061847794).

Local vs. GitHub agree: clean.

@robertocarlous could you re-review and formally approve this PR at 837ce49? It's ready to enqueue apart from the peer approval.

Your last check (your comment of Oct 26) was at 6493426, and both of your formal reviews were dismissed when new commits landed. Since then:

• bec5dd0: new caching in DINTaskCoordinator.finalizeT2Aggregation (batchId / aggregators.length), mirroring finalizeT1Aggregation. This is contract code no peer has reviewed yet.
• 837ce49: merges develop (ae7e8d6). It resolves the StakingEnforcement.t.sol import conflict and removes the duplicated TC_GINotEnded / TC_SlotsAlreadyReleased from DINShared.sol, since develop now declares them.

Diff to review: git diff 6493426 837ce49 -- foundry/

foundry/src/** needs a peer approval plus a second sign-off (MAINTAINING No. 4.2). My approval covers the second sign-off, so yours is the one missing. CI OK is green on 837ce49, and GitHub reports it mergeable.

@umeradl The peer check refuses me on this one — I have a co-authored commit on the branch (577e6c7, the develop-sync merge from the earlier review pass), so I can't be the peer approver (MAINTAINING No. 7).

The peer approval needs to come from @Santiagocetran or @Abidoyesimze, neither of whom have commits on this PR. Could you redirect the request to one of them?

Actual outcome: PR No. 259 merged, with two process steps skipped

This replaces the predictions in the pre-merge review comment with what actually happened.

One commit on develop:

1. 1d3b484: the merge of this PR, as a merge commit, not a squash. Its parents are develop 1207bf9 and PR head 837ce49, which is the head I approved on Nov 5. @Abidoyesimze, the author, added it to the merge queue at 16:55 UTC on Nov 6, and the queue merged it at 17:04 UTC. It applied with no conflicts, and GitHub shows the PR as MERGED.

Files unchanged from the PR (4 of 4)

git diff 837ce49 1d3b484 is empty for all four files:

• foundry/src/DINTaskAuditor.sol
• foundry/src/DINTaskCoordinator.sol
• foundry/src/DinValidatorStake.sol
• foundry/test/StakingEnforcement.t.sol

develop gained only PR No. 293 between my review (merge-base ae7e8d6) and the merge (1207bf9). PR No. 293 touches .gitignore, Documentation/technical/audits/dp-noise-multiplier-verification.md and scripts/dp_noise_multiplier_sweep.py. None of those overlap with this PR.

Verification

• Merge queue CI OK on the merge-group commit: green (run 42256939249).
• develop push run on 1d3b484: green (run 42258275515).

The code is fine on develop.

Process: two rules from Developer/MAINTAINING.md were skipped

GitHub allowed the merge because the ruleset only checks for one approval from someone who didn't push last, plus a green CI OK. Two rules in MAINTAINING.md aren't enforced by GitHub, and this merge skipped both:

1. No. 1, "Nobody approves, merges or closes their own work": under that rule, "Merge a PR into develop" is for "the peer who reviewed it, or Umer", never the author. Adding the PR to the merge queue counts as merging it: it's the Merge when ready step in No. 4.4 (A), and it belongs to the reviewer.
2. No. 4.2, two keys for foundry/src/**: this PR changes three contracts, so it needs a peer's review plus a second sign-off from @umeradl or @abrahamnash. My approval at 837ce49 was the second sign-off. My comment of Nov 5 said the PR was "ready to enqueue apart from the peer approval". That approval never came. @Santiagocetran hadn't reviewed the PR, and din_peer_check.py refused @robertocarlous (his reply) because he is the author of 577e6c7.

That refusal is too strict. 577e6c7 is a GitHub Update branch merge of develop, committed by web-flow, and it has no conflicts. Its tree (a7e07e8) is exactly what git merge-tree produces on its own from the same two parents. So it adds no reviewable content. MAINTAINING No. 7 treats a reviewer as a co-author when they push "substantial fixes", and a no-op merge isn't one. The script counts any authored commit, so I'll open a follow-up to make it skip merge commits whose tree matches the automatic merge. Until then, Roberto's re-review counts as the peer review here.

What happens next:

• The merge stays. It merged exactly the head I reviewed, CI is green on develop, and reverting a clean change would only add churn.
• @robertocarlous, could you do the peer review after the fact? Please review the diff at 837ce49, which is identical to what landed in 1d3b484. Post the result here. If you find anything, it goes in a follow-up PR. If you can't get to it, @Santiagocetran can do it instead.
• Issue No. 249 was closed automatically by Closes #249 when this merged. Under MAINTAINING No. 5.2 it still needs a peer's closing check (din-close-issue 249). I'll reopen it if anything turns out to be missing.
• @Abidoyesimze, for next time: once your PR is approved, leave adding it to the queue to the reviewer, or ping them to do it. This wasn't a problem with your code. The diff was good.